Skip to content

[POOL-431] Improve replenishment strategy in GenericObjectPool.invalidateObject#454

Open
atakavci wants to merge 3 commits into
apache:POOL_2_Xfrom
atakavci:atakavci/POOL-431
Open

[POOL-431] Improve replenishment strategy in GenericObjectPool.invalidateObject#454
atakavci wants to merge 3 commits into
apache:POOL_2_Xfrom
atakavci:atakavci/POOL-431

Conversation

@atakavci

@atakavci atakavci commented Apr 8, 2026

Copy link
Copy Markdown

Summary

A behavioral regression was introduced in commons-pool 2.13.0 as part of the fix for POOL-424, which ensures that invalidateObject() replaces the destroyed instance by internally calling addObject().

This PR replaces the addObject() replenishment call in GenericObjectPool.invalidateObject with a
strategy consistent with how GenericKeyedObjectPool.invalidateObject already handles this
via reuseCapacity().

// before
addObject();

// after
if (!isClosed() && idleObjects.hasTakeWaiters()) {
    try {
        addIdleObject(create(Duration.ZERO));
    } catch (final Exception e) {
        swallowException(e);
    }
}

Rationale

addObject() brings potentially unexpected behaviour other than propogating factory fails to call site: it blocks up to maxWait, silently no-ops when numIdle >= maxIdle, creates unconditionally without checking for waiters, and propagates factory exceptions — which can cause invalidateObject() to throw after the
invalidation has already succeeded.

The new approach is waiter-aware (hasTakeWaiters), non-blocking (Duration.ZERO), bypasses
the maxIdle inventory gate, and swallows factory exceptions via swallowException. Pool
state is checked upfront with !isClosed(); lifecycle management beyond that remains the
responsibility of close().

Files changed

  • src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

@atakavci
- swallow factory exceptions in invalidate
b5411d3 
- switch to addIdleObect and check takeWaiters
- synchronization to guard concurrent invalidations on same instance
garydgregory
garydgregory requested changes Apr 8, 2026
View reviewed changes

@garydgregory garydgregory left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @atakavci

Thank you for your pull request. You must add a unit test that fails without the changes to main applied. This is the only way we can avoid a future change from creating a regression.

TY!

@garydgregory

garydgregory commented Apr 10, 2026

Copy link
Copy Markdown
Member

@psteitz
WDYT?

@atakavci atakavci requested a review from garydgregory April 13, 2026 06:29
@atakavci

atakavci commented Apr 20, 2026

Copy link
Copy Markdown
Author

hey, hi @garydgregory , @psteitz ,

we 'd love to hear your feedback - happy to discuss or apply any necessary changes with the PR. thanks.

@ggivo ggivo mentioned this pull request May 8, 2026
Closed
@uglide

uglide commented May 18, 2026

Copy link
Copy Markdown

@garydgregory @psteitz Any updates on this?

ptaoussanis added a commit to taoensso/carmine that referenced this pull request Jul 16, 2026
@ptaoussanis
[fix] Pin Commons Pool at v2.12.1
bb72b22 
- Keep Commons Pool at v2.12.1 instead of v2.13.1, the latest version
  reported by `lein ancient`.

- Commons Pool v2.13.0 intentionally changed
  `GenericObjectPool.invalidateObject` for POOL-424. It synchronously
  attempts to add a replacement after invalidation to fix a
  saturated-pool liveness race involving multiple waiting borrowers;
  v2.13.1 retains that behavior.

  https://issues.apache.org/jira/browse/POOL-424

- Carmine v3 and the v4 Cluster manager use `GenericKeyedObjectPool`,
  whose waiter-aware replacement behavior is unaffected. The ordinary
  v4 pooled manager, including Sentinel, uses `GenericObjectPool` and is
  directly affected.

- With v2.13.1, ordinary v4 invalidation can create a Redis connection
  without waiting borrowers, mask the original application or I/O error
  when replacement creation fails, and leave a newly created idle
  connection after an active pre-clear connection is destroyed. The
  latter makes `_pooled-conn-manager-active-clear` observe one idle
  connection instead of zero, although the stale connection is
  correctly destroyed.

- Track the open, Major POOL-431 regression and proposed waiter-aware,
  non-blocking correction in PR #454:

  https://issues.apache.org/jira/browse/POOL-431
  apache/commons-pool#454

- Revisit the pin after POOL-431 is resolved. Before upgrading, verify
  saturated waiter liveness, no eager replacement without waiters,
  original error preservation, and pooled clear and Sentinel lifecycle
  behavior.
ptaoussanis added a commit to taoensso/carmine that referenced this pull request Jul 16, 2026
@ptaoussanis
[fix] Pin Commons Pool at v2.12.1
5509143 
- Keep Commons Pool at v2.12.1 instead of v2.13.1, the latest version
  reported by `lein ancient`.

- Commons Pool v2.13.0 intentionally changed
  `GenericObjectPool.invalidateObject` for POOL-424. It synchronously
  attempts to add a replacement after invalidation to fix a
  saturated-pool liveness race involving multiple waiting borrowers;
  v2.13.1 retains that behavior.

  https://issues.apache.org/jira/browse/POOL-424

- Carmine v3 and the v4 Cluster manager use `GenericKeyedObjectPool`,
  whose waiter-aware replacement behavior is unaffected. The ordinary
  v4 pooled manager, including Sentinel, uses `GenericObjectPool` and is
  directly affected.

- With v2.13.1, ordinary v4 invalidation can create a Redis connection
  without waiting borrowers, mask the original application or I/O error
  when replacement creation fails, and leave a newly created idle
  connection after an active pre-clear connection is destroyed. The
  latter makes `_pooled-conn-manager-active-clear` observe one idle
  connection instead of zero, although the stale connection is
  correctly destroyed.

- Track the open, Major POOL-431 regression and proposed waiter-aware,
  non-blocking correction in PR #454:

  https://issues.apache.org/jira/browse/POOL-431
  apache/commons-pool#454

- Revisit the pin after POOL-431 is resolved. Before upgrading, verify
  saturated waiter liveness, no eager replacement without waiters,
  original error preservation, and pooled clear and Sentinel lifecycle
  behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garydgregory garydgregory Awaiting requested review from garydgregory

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@atakavci @garydgregory @uglide